-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix patch for new rnmapbox-maps version #41556
Conversation
@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@francoisl I think the testing steps don't make sense to test this change because those steps are for testing the patch fixing a crash in the WebView (not the map). I would say the testing steps should be something like:
- Remove
node_modules
- Run
npm i
and make sure that the warning is gone - Create a Distance money request and check that the map works fine.
Regarding the patch fix, I'm pretty new in this, my way to verify that just renaming is enough would be to look at what the patch is doing and see if something doesn't apply anymore considering the code changes from version 10.1.11 to 10.1.12:
rnmapbox/maps@v10.1.11...v10.1.12
android/src/main/java/com/rnmapbox/rnmbx/components/camera/RNMBXCamera.kt is the only file the patch is changing that had a difference between versions 10.1.11 to 10.1.12, but the file kept the same number of lines and the patch is modifying something below the changes, so it should be fine.
I removed node_modules
and ran npm i
, I didn't get the warning:
> [email protected] postinstall
> scripts/postInstall.sh
patch-package 8.0.0
Applying patches...
@onfido/[email protected] ✔
@react-native/[email protected] ✔
@react-native/[email protected] ✔
@react-native/[email protected] (1 onStartReched) ✔
@react-native/[email protected] (2 osr-improvement) ✔
@react-native-camera-roll/[email protected] ✔
@react-native-community/[email protected] ✔
@react-native-community/[email protected] ✔
@react-native-community/[email protected] (1 initial) ✔
@react-native-community/[email protected] (2 turbomodule) ✔
@react-native-firebase/[email protected] ✔
@react-native-firebase/[email protected] ✔
@react-native-firebase/[email protected] ✔
@react-native-firebase/[email protected] ✔
@react-navigation/[email protected] ✔
@react-navigation/[email protected] ✔
@react-navigation/[email protected] (1 initial) ✔
@react-navigation/[email protected] (2 dontDetachScreen) ✔
@rnmapbox/[email protected] ✔
@shopify/flash-list/[email protected] ✔
@shopify/[email protected] ✔
[email protected] ✔
[email protected] ✔
[email protected] ✔
[email protected] ✔
[email protected] (1 initial) ✔
[email protected] (2 disableViewRecycling) ✔
[email protected] ✔
[email protected] ✔
[email protected] (1 initial) ✔
[email protected] (2 SuspenseFix) ✔
[email protected] (3 iOSFontResolution) ✔
[email protected] (4 AndroidModalSize) ✔
[email protected] (5 TextInputs) ✔
[email protected] (6 Codegen) ✔
[email protected] (7 disableTextInputRecycling) ✔
[email protected] (8 checkForHashMap) ✔
[email protected] (9 properEventDispatchOrder) ✔
[email protected] (10 resetAutoresizingOnView) ✔
[email protected] (11 optionalViewRecycling) ✔
[email protected] (12 disableNonTranslucentStatusBar) ✔
[email protected] (13 exposePrefabs) ✔
[email protected] (14 fix-inverted-flatlist) ✔
[email protected] (14 fixPath) ✔
[email protected] (14 iOSCoreAnimationBorderRendering) ✔
[email protected] (15 fixIOSWebViewCrash) ✔
[email protected] ✔
[email protected] (1 initial) ✔
[email protected] (2 turbomodule) ✔
[email protected] (1 allowedMimeTypes) ✔
[email protected] ✔
[email protected] ✔
[email protected] (1 initial) ✔
[email protected] (2 turbomodule) ✔
[email protected] ✔
[email protected] (1 fix-boost-dependency) ✔
[email protected] (2 copy-state) ✔
[email protected] (1 fix-screen-type) ✔
[email protected] ✔
[email protected] ✔
[email protected] ✔
[email protected] (1 initial) ✔
[email protected] (2 measureInWindow) ✔
[email protected] (4 fixLastSpacer) ✔
[email protected] (5 image-header-support) ✔
[email protected] (6 fixPointerEventDown) ✔
[email protected] (7 osr-improvement) ✔
[email protected] ✔
[email protected] ✔
up to date, audited 47 packages in 338ms
5 packages are looking for funding
run `npm fund` for details
found 0 vulnerabilities
added 3502 packages, changed 2 packages, and audited 3505 packages in 1m
396 packages are looking for funding
run `npm fund` for details
33 vulnerabilities (20 moderate, 7 high, 6 critical)
To address issues that do not require attention, run:
npm audit fix
To address all issues possible (including breaking changes), run:
npm audit fix --force
Some issues need review, and may require choosing
a different dependency.
Run `npm audit` for details.
The map seems to be working fine:
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.71-6 🚀
|
@aldo-expensify for context- can you review please? I'm not super clear on the test steps we need for this, I reused the tests from this PR but I don't see how they apply to rnmapbox.
Details
This fixes the following warning when you run
npm i
on main:because this PR bumped the version of rnmapbox, without changing the patch that was introduced in #41023.
I generated the new patch with
npx patch-package @rnmapbox/maps
, it's identical as the existing one.Fixed Issues
N/A - explained above
Tests
npm i
patch-package
Offline tests
N/A
QA Steps
accountingOnNewExpensify
)PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop